-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: issue-5850 - Settle override conflicts between edges and propagate changes #7025
base: latest
Are you sure you want to change the base?
Conversation
This is going to need tests |
The edge's |
There are several bugs in the mechanism, and this is just the first fix. |
@wraithgar |
@wraithgar Anything else we need to do to merge this? |
@AlonNavon This looks awesome, really looking forward to having overrides work more the way you'd expect. With this change, will running |
@sgarfinkel The major bugs:
|
Just patience. Holidays are over and we got a lot of PRs. This PR is on the work board it's not been forgotten. |
Why was this closed? |
The PR wasn't made from the OP's fork, but presumably from their company's (which means that maintainers can't push to the PR branch, btw - always only make PRs from your own personal forks), and presumably someone at their company deleted the fork. |
Yeah, this was a mistake on our end. Our devops deleted the repo by accident. |
Can we have an update on this please? The PR seems to be on pause for months, but it adds important bugfixes. |
@AlonNavon @ljharb What is the status on this PR? Can we please finally merge it? It's been 2,5 years since the original issue (#4232) , and we still have to perform complete clean |
Adding my voice to the "this would be great to land". Update: @AlonNavon thank you for digging into this! This is not a simple thing 😵 |
@jdalton @jakubmazanec @jbjhjm Several months later, after the issue still went unresolved, I dove deep into the code and found exactly what's broken in it (the description at the header is the tip of the iceberg). I've done numerous fixes and got it to work properly on numerous test cases. But right now except forking npm I'm not sure what to do... |
Thanks @AlonNavon for making this PR, it takes quite a bit of time and effort to fix these kinds of bugs. Arborist is a very complicated piece of npm. It's unfortunate that this PR has sat so long, but this has proven to require more focused attention than we were able to give it. It also of course fell through the cracks as time went on, and as other priorities demanded attention. External PRs are the lifeblood of npm, and having them sit like this is does nobody any good. So, again, thank you for your time on this and for continuing to bring it back to our attention. Yes, overrides needs work. That you are willing not only to fix an immediate problem but are willing to work on this in phases to fix things more holistically is, frankly, wonderful. Doubly so given that you hadn't received much feedback in the interim. Thank you for sticking with it. Thank you also @jdalton for offering to help. It would be most welcome. Currently we are working on getting npm 11 started, so that we don't fall behind Node.js's release cycle. It is a very conservative release, mostly cleanup of old code whose removal constitutes a breaking change. And of course there is the engines update which allows us to keep our dependencies up to date. There is also a growing list of external PRs that we need to review. Any PR that is beyond a simple bug fix does take some dedicated time and focus, and that is a precious commodity. This bug fix is important, fixing overrides is important. So let's get in to what will help this land. First off, the code at a glance looks fine. What gives us the most pause is the large chunk of net new code in Arborist. This is by necessity something that we have to take more care with than a typical PR. The tests for Arborist aren't the strongest: they are mostly unit tests with not a lot of them interacting w/ the module as a whole. Side effects usually are a problem with these PRs. Given that y'all are willing to stick with this makes it easier to accept, because the thing we are mostly trying to avoid is tech debt that nobody is going to have capacity to go address. Second off, having another party willing to help review helps. I think if we get to a point where the PR author, @jdalton, and myself think things look good we can land it. Finally, I would appreciate a more specific callout of what you need from me. I know it's probably frustrating to be given MORE work here, especially since you've put so much into this already. Unfortunately that's the reality we're dealing with. This isn't something I'm going to be able to focus on deeply enough to know what you need from me. So let's start here:
Please Thanks to all who have interacted with this issue, even if it was a simple upvote. |
We would of course prefer that the majority of our work happen in GitHub issues and PRs, but if you feel you need to talk in a context that is more high bandwidth (but also more time constrained) you can find us in the |
return first | ||
} | ||
} | ||
console.log('Conflicting override sets') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wraithgar This case means that the same node has two edges with completely different override sets pointing to it. This log is useful for debugging, not sure if just to remove the line or there's some other convention used in the arborist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
return false | ||
} | ||
// This is an error condition. We can only get here if the new override set is in conflict with the existing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wraithgar This case means that the same node has two edges with completely different override sets pointing to it, which means that this node can't really decide which one is correct.
Our options are:
- Go with the flow - in the current logic the out edges continue with the first overrides set they got, so this isn't breaking comparing to the existing logic. But it's obviously the wrong behavior.
- Raise an error - just fail the command. Not sure how you do that in the arborist.
- Duplicate the node - this is the same package with different override requirements. The proper logic is to duplicate the node, and then dedup it later if there's no conflict.
Currently I chose (1), but maybe it should at least show some kind of warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A hard-won lesson we learned with peer dependencies is that 1
is not the right choice here. npm 6 can install completely invalid trees, and this was fixed in npm 7. --legacy-peer-deps
can turn the behavior back on, but again it will install trees that have incorrect peer dependencies if there was a conflict. npm should never guess the user's intent.
2
is the easiest solution, especially given that 3
may eventually fail if we end up with an already duplicated node with conflicting override requirements.
So, unless there's a really elegant way to do 3
that does't require an extra mountain of effort, 2
is best here. We'll throw with as much context as possible to help the user build a working override set, but we won't build an invalid tree.
This also leaves the door open later to implementing 3
, since we can gradually add smaller, specific cases where 3
will work, and the fallback to 2
will already be in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdalton |
@AlonNavon Emailed. Yes, lets sync up 😸 Update:
|
Update: Have since landed a variation of the fix in our |
Read this after typing my reply in defense of throwing on conflicts. Good to hear there may be (relatively) easy follow ups after this. |
A first step in fixing the overrides feature. In this PR I'm aiming to fix 3 bugs:
So if we have dependency chains A->B->C and A->C, and we override C under B, then C will be overridden.
References
Fixes some of the override issues.